Simplify default config_name and job_cls mechanism#366
Conversation
lesteve
changed the title
guillaumeeb
left a comment
guillaumeeb
left a comment
There was a problem hiding this comment.
Okay, this made me a headache to properly understand all the implications here.
I've no strong opinion on this one. I feel it makes the code a little more complex, but this is probably cleaner and may avoid some issues in the future.
| if config_name is None: | ||
| config_name = getattr(type(self), "config_name") | ||
| if config_name is None: | ||
| default_config_name = getattr(type(self), "config_name", None) |
There was a problem hiding this comment.
I'm not sure to understand why this is needed?
Why don't we just test if self.config_name is None?
| self.status = "created" | ||
|
|
||
| default_job_cls = getattr(type(self), "job_cls", None) | ||
| self.job_cls = default_job_cls |
There was a problem hiding this comment.
Same as above, not sure to understand, but I probably lack some understanding of Python.
There was a problem hiding this comment.
Hmm, I think I understand seeing you've deleted the class attribute job_cls from JobQueueCluster.
I'm not sure this helps, I still need some insights of why you prefer it this way.
| "- set the 'config_name' class variable to a non-None value\n" | ||
| "- create a section in jobqueue.yaml with the value of 'config_name'\n" | ||
| "If that is not the case, please open an issue in https://github.com/dask/dask-jobqueue/issues." | ||
| "Nope your job class needs to have a config_name class attribute" |
There was a problem hiding this comment.
Are you sure this comment is better?
| if config_name: | ||
| if interface is None: | ||
| interface = dask.config.get("jobqueue.%s.interface" % config_name) | ||
| default_config_name = getattr(self.job_cls, "config_name", None) |
There was a problem hiding this comment.
So the key is here, all the informaton is in the Job class, not in the JobQueueCuster one.
I agree this is the kind of PR where it may be easier to look at the code, rather than the diff, because the diff is quite tricky to follow. My thinking behind this was to simplify the default value mechanism for both Previously (i.e. in master before this PR is merged):
I am hoping that this PR cleans up a few things. This could probably be cleaned up further so suggestions more than welcome ! |
|
Would have need this explanations!
I understand the idea, but it feels slightly weird. This is a really small concern though, I'm happy to keep it so as I've no good suggestion to offer. |
guillaumeeb
left a comment
guillaumeeb
left a comment
There was a problem hiding this comment.
@lesteve, it's OK for me to merge this one.
|
OK let's merge this one then! |
2 participants
Supersedes and closes #361.
Fixes #358.
The idea is to use a similar mechanism for
config_nameandjob_cls(a class variable is the default value, but it can also be passed as a parameter).There are also some cleaning up of a few quirks that remained after #307.